Skip to content

net/http: dispatch the client through its RoundTripper (fetch on js/wasm) - #83

Open
0pcom wants to merge 1 commit into
tinygo-org:mainfrom
0magnet:client-transport-dispatch
Open

net/http: dispatch the client through its RoundTripper (fetch on js/wasm)#83
0pcom wants to merge 1 commit into
tinygo-org:mainfrom
0magnet:client-transport-dispatch

Conversation

@0pcom

@0pcom 0pcom commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #66.

A Client with no explicit Transport called the package-level dialing roundTrip directly, so the build-tagged Transport.RoundTrip was never consulted — on js/wasm every request died with Netdev not set instead of using the fetch API. Client.Do also short-circuited an explicit Transport past the cookie jar, header initialization, and timeout handling.

This restores the upstream shape:

  • send takes a RoundTripper
  • Client.transport() falls back to DefaultTransport, as its docs already promised
  • Do always goes through do/send, so an explicit Transport gets cookies and timeouts too
  • a nil Transport and nil DefaultTransport reports http: no Client.Transport or DefaultTransport like upstream

The netdev path is unchanged: on !js targets DefaultTransport.RoundTrip (roundtrip.go) is the same package-level roundTrip as before.

Tests: three unit tests cover default dispatch, explicit dispatch, and the nil/nil error. Verified behaviorally on js/wasm under node — http.Get with a nil Transport now reaches the fetch transport (it reports its own no-fetch-API fallback error under node, as upstream Go does) instead of Netdev not set.

The client called the package-level dialing roundTrip directly, so a
Client with no explicit Transport never reached the build-tagged
Transport.RoundTrip — on js/wasm every request died with "Netdev not
set" instead of using the fetch API (tinygo-org#66). Client.Do also short-circuited
an explicit Transport past the cookie jar, header initialization, and
timeout handling.

send now takes a RoundTripper, Client.transport() falls back to
DefaultTransport as documented, and Do always goes through do/send. The
netdev path is unchanged: on !js targets DefaultTransport.RoundTrip is
the same package-level roundTrip as before.

Verified on js/wasm under node: http.Get with a nil Transport now
reaches the fetch transport (its no-fetch-API report) instead of
"Netdev not set".
@yohimik

yohimik commented Sep 6, 2026

Copy link
Copy Markdown

This overlaps the redirect loop in #72. I checked a Git merge of e12bf8a and your 13d50d3 and found a conflict in http/client.go. The combined client must retain redirects and sensitive-header rules while routing each request through the selected RoundTripper. Default and explicit transports should both exercise redirects and cookie handling. I have kept the tested fork candidate unchanged and have not opened a duplicate PR or changed your branch.

@0pcom

0pcom commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Agreed the two must compose, so the composition is now built and tested rather than argued: main...0magnet:net:client-transport-dispatch-72 is your e12bf8a with this PR's commit rebased on top.

The conflict resolves one way that makes sense: Do loses both the c.Transport short-circuit and the comment #72 adds beside it (the comment stops being true once c.send reads the transport), do's redirect loop stays exactly as #72 wrote it, and each hop's c.send dispatches through c.transport(). Your roundTrip Host fallback stays live on the default path — on non-js targets DefaultTransport.RoundTrip is that same package-level dialer.

The combined branch also adds the coverage you named: a redirecting fake RoundTripper driven through both an explicit Transport and a swapped DefaultTransport (two hops observed on each), and a cross-host vs same-host redirect pair asserting the Authorization header is stripped and kept respectively — that pair exercises makeHeadersCopier and shouldCopyHeaderOnRedirect through the dispatch. All pass with the dev compiler (tinygo test net net/http), and a js/wasm client build still compiles. Those tests only make sense once both PRs exist, so they live on the combined branch, not here.

Proposed order: #72 lands first — it is older and carries the bulk — and this PR then rebases to the combined branch above, which is ready to fast-forward. If a maintainer merges this one first instead, the same resolution applies from the other side and either of us can push it.

@yohimik

yohimik commented Sep 6, 2026

Copy link
Copy Markdown

Thank you. The proposed order works for our series: merge #72 first, then rebase #83 with the combined resolution. I checked combined head 423aacc3c90dff2cb2551c88609b112d2e8f0ea5. It retains the redirect loop and Host fallback and selects the transport in c.send.

One check before I repeat the test claim: that head contains only TestClientUsesDefaultTransport, TestClientUsesExplicitTransport and TestClientNoTransport in http/clienttransport_test.go. I could not find the redirecting transport or same-host/cross-host header tests described above. Are those tests still local, or is there another commit to fetch? Please include the cookie-jar redirect case for both default and explicit transports as well.

The published fork remains unchanged. I have not run the combined branch or counted its results as release evidence. #72 remains the redirect change and #83 remains your transport change; no duplicate PR is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

net/http: client always uses TinyGo roundTrip instead of roundTrip_js in wasm

2 participants